Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate symbol and klabel #4045

Merged
merged 23 commits into from
Jul 4, 2024
Merged

Deprecate symbol and klabel #4045

merged 23 commits into from
Jul 4, 2024

Conversation

Baltoli
Copy link
Contributor

@Baltoli Baltoli commented Feb 26, 2024

This PR implements @tothtamas28's warning messages for usages of the deprecated klabel(_) and symbol attributes (which can now be unambiguously replaced by symbol(_) and overload(_)).

The actual code changes are pretty minimal, but we need to do a bunch of updates on test cases and related output to keep tests passing. Additionally, we have pre-merged downstream PRs to make sure other projects are not subjected to noisy compiler output as a result of making this change:

Downstream updates:

@Baltoli
Copy link
Contributor Author

Baltoli commented Feb 26, 2024

Blocked on #4036

Blocked on runtimeverification/haskell-backend#3742

@Baltoli Baltoli self-assigned this Mar 14, 2024
@Baltoli Baltoli force-pushed the deprecate-symbol-klabel branch from fec6c79 to 2495800 Compare March 14, 2024 11:04
rv-jenkins pushed a commit to runtimeverification/haskell-backend that referenced this pull request Mar 20, 2024
)

See #3742 for full context; this PR adds a backwards-compatible check
for `symbol'Kywd` as well as `klabel` for the pseudo-builtin sorts
`Endianness` and `Signedness`. When this is merged, I'll be able to
finish working on runtimeverification/k#4045.
Then, when that PR lands, the frontend will no longer emit `klabel(_)`
to KORE, and we can safely remove the backwards-compatible part of this
PR.

The integration test suite's checked-in KORE files are all updated to
use the new attribute rather than the old one, but the tests that use K
to compile fresh KORE will still be using `klabel(_)` until the
dependency update job induced by the changes above goes through.

I have verified manually that building K with this HB commit allows the
failing integration test in #3742 to pass.

Thanks @jberthold for the suggestions!

~~The implementation here would ideally be made backwards-compatible; my
version induces a dependency loop (hence failing tests, it would need an
update to the K dependency to go through first for the integration test
suite to pass).~~
@Baltoli
Copy link
Contributor Author

Baltoli commented Mar 26, 2024

Blocked on: #4129

rv-jenkins pushed a commit that referenced this pull request Mar 26, 2024
When working on #4045, I identified a case in the C Semantics where an
overload set was flagged as being singletons (that is, that the
productions in the set were not actually overloads of one another).
Despite this warning, removing the `overload(_)` attributes broke
compilation by producing a parsing ambiguity.

It turns out that the issue was a difference between the main module and
the _disambiguation module_ produced during the inner parsing process.
The disambiguation module adds an additional production
```
Es ::= E
```
for user list sorts `Es` that subsorts the list element sort into the
list sort. This means that two productions
```
S ::= f(Es)
T ::= f(E)
```
can be overloads of one another.

However, this check was only using the overloads induced by the main
module and not those used by the disambiguation module. The fix is
simply to compute the disambiguation module[^1] and use its overloads
for the checks. Doing so requires some changes to the plumbing; the
subsorting production is previously only added when `RULE-LISTS` is
imported, but we need to short-circuit that code path in this context.

This PR includes a regression test minimised from the original example.

[^1]: I tried a cheaper fix that just checks parameters of
overload-attributed productions to see if they appear in a user list,
but this isn't sufficiently general in the case where the overload
parameter is actually a sub-/supersort of the user list element, as
happens in the C semantics.
rv-jenkins pushed a commit that referenced this pull request Mar 27, 2024
When working on #4045, I noticed that a pair of productions like:
```k
syntax KItem ::= disambiguate(KItem, KItem) [overload(disambiguate)]
               | disambiguate(List,  Set)   [overload(disambiguate)]
```
were both being incorrectly flagged as singleton overloads, despite
meeting the criteria for being an overload set.

Some investigation reveals that this is due to the overload lattice
checks being applied _before_ the compiler inserts subsorting
productions into `KItem` for every sort; we therefore don't know that
`List <: KItem` when constructing the overload lattice.

The solution is to move the overload checks later in the compilation
process, after the compilation pipeline has been run. This PR makes that
change, along with some associated documentation and renaming for
clarity. It also includes a regression test with the example above; I
have verified that the test breaks before this PR.
@Baltoli Baltoli marked this pull request as ready for review July 3, 2024 16:00
@Baltoli Baltoli force-pushed the deprecate-symbol-klabel branch from 0bf16a2 to 60ad675 Compare July 3, 2024 16:05
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@rv-jenkins rv-jenkins merged commit 9941e87 into develop Jul 4, 2024
18 checks passed
@rv-jenkins rv-jenkins deleted the deprecate-symbol-klabel branch July 4, 2024 09:42
@tothtamas28 tothtamas28 mentioned this pull request Jul 8, 2024
rv-jenkins pushed a commit that referenced this pull request Jul 10, 2024
Related:
* #4045
* runtimeverification/kontrol#679

Changes:
* Remove `Atts.KLABEL`
* Make `Atts.SYMBOL` unary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants